[reconfigurator] background task for marking VMMs to be stopped for update - #11170
[reconfigurator] background task for marking VMMs to be stopped for update#11170karencfv wants to merge 12 commits into
Conversation
|
@sunshowers I have a question for you. This is the first time I write a rendezvous subtask so I might just be missing some context. In #11127 I added a After reading https://rfd.shared.oxide.computer/rfd/0541#_proposal_reconciliation_rpw and taking a look at #11115 , I understand that I shouldn't have added the column to that table at all and instead I should have created a separate rendezvous table. You suggested using a rendezvous subtask, but the thing is, I don't think we can follow that approach here. We do want the In addition, https://rfd.shared.oxide.computer/rfd/0541#_creating_rows_in_rendezvous_tables the resource exisiting in inventory. I don't think VMMs should be collected as part of an inventory collection, there are just too many. I think I'd rather go with a background task, thoughts? |
|
I think it's fine for rendezvous subtasks to not only write to a rendezvous table, though I'll let @smklein and @davepacheco chime in. |
karencfv
changed the title
|
Update: We had a chat about this in the update watercooler and decided to go with a normal background task. The task should get which sleds to mark from the rendezvous table though. |
| .filter(dsl::state.eq_any([ | ||
| DbVmmState::Creating, | ||
| DbVmmState::Starting, | ||
| DbVmmState::Running, | ||
| DbVmmState::Rebooting, | ||
| ])) |
There was a problem hiding this comment.
@hawkw I'd like to get your input on whether these are the correct states the VMM should be in, in order to mark it to be stopped
There was a problem hiding this comment.
Definitely want to defer to Eliza about the specific states, but regardless of the answer today, we probably want to put this in a match somewhere so we have to consider new states added in the future? Maybe something like a DbVmmState::stoppable_states() or something that has an explicit match over all variants?
| /// This is an emergency lever for support / operations. It should only be | ||
| /// necessary if something has gone extremely wrong. |
There was a problem hiding this comment.
I don't think the nexus config is really something support / operations can control - it's not persistent if the sled (or Nexus zone) restarts, and requires manually bouncing the service within the zone for it to take effect.
If we need an emergency stop for support, I think we need a config in crdb that can be toggled via omdb, like the controls we have on the blueprint planner? If having an easy way to enable/disable this task between releases is all we need, then putting it here is great.
| .filter(dsl::state.eq_any([ | ||
| DbVmmState::Creating, | ||
| DbVmmState::Starting, | ||
| DbVmmState::Running, | ||
| DbVmmState::Rebooting, | ||
| ])) |
There was a problem hiding this comment.
Definitely want to defer to Eliza about the specific states, but regardless of the answer today, we probably want to put this in a match somewhere so we have to consider new states added in the future? Maybe something like a DbVmmState::stoppable_states() or something that has an explicit match over all variants?
|
|
||
| let updated = diesel::update(dsl::vmm) | ||
| .filter(dsl::time_deleted.is_null()) | ||
| .filter(dsl::stop_for_update_disposition_generation.is_null()) |
There was a problem hiding this comment.
Just confirming I understand: for a given VMM row, this column can only ever go from NULL to a single non-NULL value, which puts it in a terminal state of "needs to be stopped", right? It can never go back to NULL nor do we ever need to change the specific generation value once it has one?
| } | ||
| }; | ||
|
|
||
| if vmms_marked > 0 { |
There was a problem hiding this comment.
If we marked any VMMs, are there any other bg tasks we should activate as a result? (Or will there be in the future?)
| .select(rz_dsl::update_disposition_generation) | ||
| .single_value(), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
I diesel::debug_query()'d this to see what SQL it's running, and that gave me this:
UPDATE "vmm"
SET "stop_for_update_disposition_generation" = (
SELECT "rendezvous_sled_bp_availability"."update_disposition_generation" FROM "rendezvous_sled_bp_availability"
WHERE
"rendezvous_sled_bp_availability"."sled_id" = "vmm"."sled_id"
AND "rendezvous_sled_bp_availability"."bp_availability" = 'unavailable'
LIMIT 1
)
WHERE
"vmm"."time_deleted" IS NULL
AND "vmm"."stop_for_update_disposition_generation" IS NULL
AND "vmm"."state" = ANY('creating', 'starting', 'running', 'rebooting')
AND "vmm"."sled_id" = ANY(
SELECT "rendezvous_sled_bp_availability"."sled_id" FROM "rendezvous_sled_bp_availability"
WHERE "rendezvous_sled_bp_availability"."bp_availability" = 'unavailable'
)That looks correct, I think, but is pretty complicated and contains two subqueries. I think this is equivalent with no subqueries:
UPDATE vmm
SET stop_for_update_disposition_generation = r.update_disposition_generation
FROM rendezvous_sled_bp_availability AS r
WHERE r.sled_id = vmm.sled_id
AND r.bp_availability = 'unavailable'
AND vmm.time_deleted IS NULL
AND vmm.stop_for_update_disposition_generation IS NULL
AND vmm.state IN ('creating', 'starting', 'running', 'rebooting')I tried hitting both of these with EXPLAIN, and the output was pretty similar, so maybe cockroach is doing a good job of turning the former into the latter? I don't think diesel supports UPDATE ... FROM ..., so to get the latter we'd have to use one of the raw query builder gadgets. I'm not sure that's worth it.
One other note from the EXPLAIN: both of these queries induce a FULL SCAN over the vmm@lookup_vmms_by_sled_id partial index. I think it's only because this is a partial index that the query is allowed, but that partial index is still going to be "all non-deleted VMMs on all sleds". Do we need to figure out how to paginate this? EDIT: This is "all non-deleted VMMs on all evacuating sleds", which is a much smaller number. This is probably fine!
| Vmm { | ||
| id: Uuid::new_v4(), | ||
| time_created: Utc::now(), | ||
| time_deleted: None, |
There was a problem hiding this comment.
Is it worth confirming we don't touch VMMs with a non-NULL time_deleted?
| // Sleds A and B are both evacuating (`unavailable`), at different | ||
| // generations, and sled C is available. In a single pass the stoppable | ||
| // VMMs on both sled A and sled B should be marked, each at their own | ||
| // sled's generation, regardless of which generation that is. |
There was a problem hiding this comment.
Assuming I understood correctly in https://github.com/oxidecomputer/omicron/pull/11170/changes#r3883069906, should we confirm that if one of these sleds becomes available again, its VMMs remain marked, and if it then becomes unavailable with a higher generation, the VMMs we marked the first time keep their original generation?
3 participants
Related: #11169